fix: Add more typecheck fixes for CLI parse - #5577
Conversation
Card code that imports `@cardstack/runtime-common`, and any card whose `@model.<field>` value type is derived through the `primitive` unique symbol, failed to type-check in a published install: runtime-common is a devDependency npm doesn't install, so the bare specifier was unresolvable and the field-value mapping collapsed to the field class. Generate runtime-common's `.d.ts` into `bundled-types/` (plain emission — no content-tag or `: any` annotation, so the `primitive`/`realmURL` `unique symbol` identities survive) and alias `@cardstack/runtime-common` + `/*` to it in the parse tsconfig. Promotes the plain-glimmer and runtime-common fixtures to the must-pass set; the decorator and positional -helper gaps remain deferred. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The helpers-and-fields fixture called `(formatDateTime @model.when 'MMM D')` — a positional format the helper does not accept: its signature takes the date positionally and options (including `format`) as named args, matching the documented usage and the runtime, which takes an options object. The npm-installed glint correctly rejected the positional shape (the monorepo's leniently allowed it). Use `format='MMM D'` and promote helpers-and-fields to the must-pass set; only the decorator gap remains deferred. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A decorator (`@tracked`, `@field`) on a member of a format-class
expression (`static isolated = class { … }`) can't type-check under
TypeScript's legacy decorators, which allow decorators only on class
declarations — so glint reports the cryptic "Decorators are not valid
here". The code runs, but the type-check can't pass, and the raw message
doesn't say what to change.
`boxel parse` already surfaces the diagnostic with its file and line
(it isn't silently swallowed); augment the TS1206 message with the fix:
move reactive state into a top-level component the format class renders.
The tracked-format-class fixture now asserts that guidance rather than
standing as a deferred it.fails.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`boxel parse` symlinked a single node_modules into its temp type-check workspace. That breaks when an install splits the CLI's deps across two dirs: a consumer that pins an incompatible @glint/ember-tsc makes npm nest the CLI's copy while other deps (@glimmer/*, qunit) stay hoisted at the install root, so no single dir holds them all and linking one hides the rest. Resolve each required package from the CLI's location and symlink them individually. `resolvePackageRoot` walks node_modules up the ancestor chain like Node's own resolver, so it finds each package whether nested or hoisted (and, unlike require.resolve, finds type-only `@types/qunit`). The monorepo-dev path still single-symlinks host's node_modules, which holds the full set in one place. Verified against a constructed split tree (root @glint 1.4.0, CLI's 1.5.0 nested, @Glimmer hoisted): all parse fixtures resolve and type-check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dec66fc174
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed the published-install dep resolution and the runtime-common .d.ts bundling, tracing each new path against a real npm layout rather than the monorepo — plus checking the two fixture/test changes are actually correct, not just green.
Bottom line: the design is sound and the diagnostics work is a genuine improvement, but two resolution gaps keep the published parse from doing what this PR intends — one blocking (the missing @universal-ember/test-support link), one silent (dropped .mts). Both were flagged by Codex; I verified each reaches real card code and left the mechanism + exact fix on its thread. No other defects found.
What lands right (verified, not skimmed):
- The split-tree diagnosis is correct and
resolvePackageRootis the right shape — it mirrors Node's ancestor-chain resolution and, unlikerequire.resolve, finds type-only@types/qunit(no runtime entry). Keeping the single whole-node_modulessymlink on the monorepo path is a good call. - Bundling runtime-common through the real
.d.tspipeline (plain emission, no: any) is what preserves theprimitive/realmURLunique symbolidentities that card field-value typing depends on — theplain-glimmerandruntime-commonfixtures now pin exactly that. - The
formatDateTimefixture fix is correct: the helper signature isPositional: [DateLike],Named: FormatDateTimeOptions(which carriesformat), soformat='MMM D'is the only valid shape — the old second-positional call was genuinely wrong, the npm-glint rejection was right, and the monorepo's acceptance was the lenient one. - The TS1206 explainer is wired correctly (
tsCodeis captured asTS\d+and matched as'TS1206'), and the test pins file/line attribution, not just message text. Turning anit.failsdeferral into asserted guidance is a real upgrade.
Recommendations:
- Blocking: add
'@universal-ember/test-support'toGLINT_RESOLVE_PACKAGES, plus a fixture that imports it so CI guards the path (detail on the P1 thread). - Non-blocking (silent correctness): accept
.mts/.ctsin both the walk filter and the generated tsconfiginclude, somarked.mtsemits and the bfm/marked.d.tsstop dangling (detail on the P2 thread).
Adjacent, out of scope: runtime-common is bundled wholesale (minus tests); if published bundle size matters, a follow-up could prune modules unreachable from the card type surface — not this PR's job.
Generated by Claude Code
The split-tree fix linked a hardcoded subset of packages, which silently dropped card-facing deps the old whole-node_modules symlink exposed — first @types/qunit, now @universal-ember/test-support (imported by realm .test.gts files via getService). Each omission is a published-install-only "Cannot find module", invisible to CI because no fixture exercised it. Derive the linked set from the CLI's own package.json `dependencies` so it can't drift: every dependency the CLI declares (each added precisely so card code / .test.gts can resolve it) is linked automatically. Add a `.test.gts` fixture importing @universal-ember/test-support to the must-pass set so the test-support path is exercised. Verified against the split tree (root @glint 1.4.0, CLI's 1.5.0 nested, @Glimmer hoisted): all fixtures, including test-support, type-check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The runtime-common .d.ts build dropped `marked.mts` — the walk filter required `.ts`, and the tsconfig `include` glob `**/*.ts` matches neither `.mts` nor `.cts`. Exported modules (bfm-math, bfm-card-references, marked-sync) import from it, so their emitted declarations kept dangling `./marked.mts` references. Under parse's skipLibCheck that doesn't error — the affected types (TokenizerAndRendererExtension and anything built on it) silently collapse to `any`, weakening the very check the bundle exists to strengthen, and card-api reaches these via `@cardstack/runtime-common/bfm-card-references`. Accept `.mts`/`.cts` in the source walk and add `**/*.mts` / `**/*.cts` to the emit `include`. `marked.d.mts` now ships and the references resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The bundled runtime-common .d.ts silently degraded three times (dropped @types/qunit, @universal-ember/test-support, marked.mts) — each a published-install-only failure that no parse fixture could catch, because skipLibCheck turns a dangling relative import into `any` rather than an error. Add `assertBundleDeclRefsResolve`: after emitting the declarations, scan each for relative `from`/`import()`/`require()` specifiers and fail the build (which runs in CI) if any resolves to a file absent from the bundle. Bare package imports and non-module assets are skipped. Verified: passes on the current bundle; reverting the .mts inclusion makes it fail loudly, naming `bfm-*.d.ts → ./marked.mts`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Re-review of the work since the last pass — verifying the two earlier findings are actually fixed (not just acknowledged) and reviewing the new machinery that replaced the hand-list: the package.json-derived dependency set and the bundle integrity check.
Bottom line: both prior findings are resolved, and I verified each empirically against CI on the current head rather than trusting the diff. The new machinery is correct and a genuine improvement over what it replaced. No new blocking issues.
P1 (test-support not linked) — resolved, and now guarded by CI. linkResolvedDeps derives its set from boxel-cli's declared dependencies (CLI_DEPENDENCY_PACKAGES), and @universal-ember/test-support is one of them, so it's symlinked into the bundled parse workspace. The new test-support fixture (a .test.gts importing getService) is promoted into CLEAN_FIXTURES, and Boxel CLI Tests runs test:cli:tarball — a packed-tarball npm install driven via BOXEL_CLI_BIN, i.e. the BUNDLED_TYPES_DIR + linkResolvedDeps path, the exact layout the regression lived in. That job is green, so the fixture type-checks in the published layout: the path is now covered, not merely patched. Deriving from package.json is strictly better than the curated list — the linked set can't silently drift from the declared deps again.
P2 (.mts dropped from the bundle) — resolved, plus a defensive backstop. The walk filter and the generated tsconfig include both accept .mts/.cts now, so marked.mts is copied and emits marked.d.mts; the ./marked.mts references in bfm-math / bfm-card-references / marked-sync declarations resolve instead of collapsing to any under skipLibCheck. On top of that, assertBundleDeclRefsResolve fails the build on any emitted declaration that keeps a relative specifier with no file behind it. I traced its candidate resolution — a ./marked.mts (or .mjs) specifier normalizes to the marked.d.mts stem candidate and resolves — and Boxel CLI Build (pnpm build → build:types → build-types.ts) is green, which means the check ran without process.exit(1): the .mts emits and nothing dangles. Making the check general rather than special-casing marked.mts is the right instinct.
One latent fragility, non-blocking: assertBundleDeclRefsResolve scans raw .d.ts text with a regex, so a preserved JSDoc comment containing an import-like string (from './x') would be read as a real specifier and could false-positive the build. Not triggering today (build is green), and cheap to live with — worth knowing if the check ever trips on a declaration whose only from './…' sits inside a doc comment.
Adjacent, out of scope: the integrity check runs only for the runtime-common bundle; base's bundle goes through its own content-tag pipeline and isn't validated the same way. Not this change's job — flagging it for whoever hardens the base bundle next.
Generated by Claude Code
2 participants
This addresses the other parse failures that #5551 added failing tests for.